Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternative box-shadow for dark theme. #8475

Closed
wants to merge 1 commit into from
Closed

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Jan 17, 2025

I've been looking into the recommendations for shadows in dark mode, and the most prominent one is just to remove them like in this article. However, another article suggested a different solution, and after a bit of experimentation, I think I like that we can have the same style for both modes.

Note: while the dark background is not really visible in dark mode, the additional opaque black shadow does create a slight contrast with the opaque white inset. To me it does feel like it has some kind of natural depth.

I am a bit undecided upon the "up" or "down" effect: in this article the "elevation" of an element is connected with the darkness and narrowness of the shadow. So if we want to keep being consistent, we want to have smaller shadow on hover (as well as darker, but that's already done). However, the current pub.dev implementation increases the shadow on hover, so I've kept it for now, but maybe we should rather go with the different direction?

image image

@isoos isoos requested a review from sigurdm January 17, 2025 17:11
@isoos
Copy link
Collaborator Author

isoos commented Jan 17, 2025

Note: a staging should have this online soon (alongside with a restore from backup).

/cc @parlough

@sigurdm
Copy link
Contributor

sigurdm commented Jan 20, 2025

Could you also add a screenshot showing the alternative (dark mode without shadows) for comparison?

@isoos
Copy link
Collaborator Author

isoos commented Jan 20, 2025

One possibility: only border:

image

Another, using box-shadow to draw solid border:

image

@sigurdm
Copy link
Contributor

sigurdm commented Jan 20, 2025

I like both.

By the way, did you read the material dark-mode guide on this?
https://m2.material.io/develop/android/theming/dark#:~:text=Shadows%20are%20less,implied%20light%20source.

@isoos
Copy link
Collaborator Author

isoos commented Jan 20, 2025

I think the remaining part from the guide may be the different surface color, but I didn't want to go there yet, only to remove the not-that-nice current implementation of the shadow.

My preference would be the contrast that the PR gives, but I also like the simple box-shadow marking the border (as in the "cron" example above). I think the border-only alternative is too flat, I'd rather use any of the other versions.

@jonasfj jonasfj requested review from szakarias and removed request for jonasfj January 22, 2025 16:11
@@ -108,14 +108,14 @@
.mini-list-item {
background: var(--pub-neutral-bgColor);
border-radius: 4px;
box-shadow: 0px 2px 7px 0px var(--pub-home_card-box_shadow-color);
box-shadow: var(--pub-box-shadow);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use something other than shadow at all? just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the linked articles: the basic suggestion is to use border + lighter background for elevating count out of its surroundings. I still find those mostly flat, and I thought the inset + minimal shadow contrast may be interesting to check out. If that is not winning here, we shall do border + lighter background (though the inset is kind of that, with slight gradient changes).

@isoos
Copy link
Collaborator Author

isoos commented Jan 23, 2025

Closing in favor of #8489.

@isoos isoos closed this Jan 23, 2025
@isoos isoos deleted the shadow branch January 23, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants